-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Meta: Clearly document current guidance for changing existing PEPs #2378
Meta: Clearly document current guidance for changing existing PEPs #2378
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the bar for Active PEPs is higher. Maybe ask @warsaw .
I have a general concern that we're straying into being too prescriptive. We don't get historical change PRs often, and regardless of this guidance we would still need to spend effort telling people when such PRs are opened in the future (as they will continue to be). I'll review the specific textual changes this weekend, but I am probably also going to request a significant shortening -- I appreciate the contributing paper is not short currently at ~130 lines, but this change adds nearly two fifths of that length in new text. A |
I've edited down the text to reduce length and prescriptiveness, particularly in the bullets summarizing the guidelines for each type of PEP (which was the actual sorta-prescriptive part). In total, this section now only adds 49 lines (including blanks) to to the 178 line document, or just above 1/4 of the total length, while removing 8 lines from the spellcheck section of text that was moved to this one, for a delta of only +41 lines, or 2/9 the total (and 4 of those are just URLs that don't render); by contrast, just the linting sections are 75 lines. This seems fairly appropriate, since this section is framed in terms of guidelines rather than hard rules, not only discusses "making changes to old PEPs", but also a number of directly related issues that have come up recently or been otherwise requested here, including:
In total, these issues do come up fairly frequently (see examples above) here and elsewhere, and while some users still may miss it anyway (though GitHub's UI does its best), it gives us a section to link to when it does (as others have requested), and contributors tend to react much better to a documented guideline they missed, than an unwritten rule they are only told after putting effort into a PR. Overall, I agree we shouldn't be overly prescriptive, but since we have these mostly unwritten rules that we're enforcing (somewhat inconsistently), I think it would be best to document them and agree on at least a set of guidelines so that contributors and ourselves have a common baseline understanding of them and know (how) to reach out to clarify for specific situations. |
CONTRIBUTING.rst
Outdated
Commit messages and PR titles | ||
----------------------------- | ||
|
||
When adding or modifying a PEP, please always include the PEP number in the | ||
commit summary and pull request title. | ||
For example, ``PEP NNN: <summary of changes>``. | ||
Likewise, prefix rendering infrastructure changes with ``Infra:``, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can skip this sentence for brevity. It's not that important that the prefixes are consistent (though I agree with you that we should avoid obscure abbreviations like "PRS").
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can, though I don't think it hurts to be provide some basic guidance here, since a lack of such was how we got confusing ad-hoc initialisms like "PRS" in the first place, and its helpful to see at a glance the general high-level topic of a commit/PR without having to parse the full title/commit message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also drop the "Lint" prefix and just have "Infra" and "Meta", if that would simplify things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Lint", "Infra" and "Meta" are good.
I'm also confused about "PRS", what does it stand for? It's very difficult to search for :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like opinions were mixed about specifying particular prefixes (@hugovk and me 👍 , @JelleZijlstra and @AA-Turner 👎 ), but us three plus Guido all found "PRS" very confusing, so I dropped this sentence for now on the condition that we all avoid bespoke initialisms like that going forward. Deal?
I'm also confused about "PRS", what does it stand for? It's very difficult to search for :)
AFAIK @AA-Turner made it up, apparently (per his responses to Guido, Jelle, and me who all asked the same question). I believe it was intended to stand for "PEP Rendering System".
I've re-read the proposed changes & my (alternate) suggestion for the entire section is: Contributing changes to existing PEPsPEPs serve as a record for why a change was made, including a detailed specification for the change. This means that in general, PEPs are considered to be historical documents. Edits to existing PEPs may be accepted, although we encourage you to contact the PEP author(s) or gather consensus for the update first. PEP 1 contains more guidance. I appreciate this is rather different from the proposal here -- my rationale is that PEP 1 already contains text on this, and given that the CONTRIBUTING.rst document is non-normative, I don't want to make promises we can't keep. A |
e266de3
to
c348b69
Compare
@AA-Turner I agree with your comments overall; it doesn't make sense to partially duplicate the relevant section in PEP 1, while not offering or directly referring to explicitly normative guidance, and it would be better to more clearly separate the purpose of the content in each, while referencing the other for the rest, and reducing the total length. I do think it makes sense to retain some existing content unique to and more appropriate for the Contributing Guide there, including specific guidance on what to make as PRs (copyediting/proofreading draft PEPs), what to direct to the Discussions-To thread (substantive changes) and what to avoid (e.g. mass typo correction PRs, which was already there before per your own request); and mentioning resources to reach out to if unsure. Also, there are a few more substantive items not currently mentioned in PEP 1, including SC approval of changes to governance PEPs, holding changes to PEPs during final review and resolution, and reviving Deferred/Withdrawn PEPs. Therefore, I went ahead with this, retaining the former existing helpful items in the Contributing Guide, revising PEP 1 to reflect those missing bits, and cutting everything else. |
I think if we’re now considering this document too formal for parenthetical remarks, we have taken a wrong turn somewhere (and not just because I’m too fond of them myself :-). |
c554bb1
to
05bc9ae
Compare
05bc9ae
to
3968613
Compare
I belatedly noticed there is also a (rather verbosely named) "Reporting PEP Bugs, or Submitting PEP Updates" section in PEP 1 that covers some similar ground to the section in the Contributing Guide, as well as some elements of what was added to the PEP Maintenance section. As such, I've made some additional changes to this PR to focus each section more tightly, reduce duplication and further shorten the added length:
Also, I rebased the PR after the implementation of PEP 676, and updated the added links accordingly. |
@python/pep-editors I've revised this one last time to further reduce duplication, cross-link the sections involved and slim down the total net additions. Any final feedback on this? Also, do you think its substantive enough to be worth submitting to the SC for assent before merging? As far as I'm aware, it only clarifies the existing de jure and de facto established practices, but I don't want to be too hasty about it. Thanks! |
LGTM. I think it's all just tightening up what's already been the case, so I don't think it should need SC approval, but it'd be nice to have at least one SC member sound off on that point. |
☝️ @brettcannon @encukou ? Also, your judgement on this question would be much appreciated. |
LGTM, other than my comment above |
I'll wait another 24 hours in case anyone has further comments or objections, and if not, I'll go ahead and merge. Thanks everyone for all the great feedback that greatly shaped and improved the final result! |
pep-0001.txt
Outdated
Informational and Process PEPs may be updated over time to reflect changes | ||
to development practices and other details. The precise process followed in | ||
these cases will depend on the nature and purpose of the PEP being updated. | ||
Active Informational and Process PEPs may be updated over time to reflect |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Active Informational and Process PEPs may be updated over time to reflect | |
Active, Informational and Process PEPs may be updated over time to reflect |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, the intended syntax is Active (Informational and Process) PEPs
, not (Active, Informational and Process) PEPs
as the above change would imply. Any suggestions to make that clearer (as that obviously wasn't what came across)? E.g.
Active Informational and Process PEPs may be updated over time to reflect | |
Active-status Informational and Process PEPs may be updated over time to reflect |
or
Active Informational and Process PEPs may be updated over time to reflect | |
Informational and Process PEPs that are Active may be updated over time to reflect |
or even as a literal parenthetical?
Active Informational and Process PEPs may be updated over time to reflect | |
Active (Informational and Process) PEPs may be updated over time to reflect |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The last one looks clearest to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I applied that
a7dc3d4
to
58e7055
Compare
Its been another week and the one additional raised (from an approving review) was resolved, so I'll finally merge this now. Thanks again for all the great feedback! |
As has come up on many issues and PRs submitted here (e.g. #2151, #2199. #2265, #2318. #2332 being but a few recent examples off the top of my head), it would be of benefit to provide clear unambiguous up-front guidance on whether, when and where to propose changes to existing PEPs. Therefore, this PR adds this near the top of the Contributing Guide (which is exposed fairly prominently by the GitHub UI), to explictly explain the established, mostly unwritten practice here.
Specifically, it:
codespell
locally #2151)Also, I added a sentence to the "Commit messages and PR titles" section clarifying prefix usage for non-PEP PR titles/commit messages, as that has been the source of some confusion lately by multiple PEP editors (e.g. on #2364 and #2375 )